rtio: add helper function `rtio_read_transaction()` #91775

avisconti · 2025-06-17T12:33:32Z

Add a helper function that constructs a rtio SQE chain with the purpose to perform a bus read operation on a list of registers.

Tested with lsm6dsv16x sensor:

I2C bus: using nucleo_h503rb board + x_nucleo_iks4a1 shield
SPI bus: using sensortile_box_pro

west build -b nucleo_h503rb samples/sensor/stream_fifo/
west build -b nucleo_h503rb  samples/sensor/stream_drdy/
west build -b sensortile_box_pro samples/sensor/stream_fifo/
west build -b sensortile_box_pro samples/sensor/stream_drdy/

(note that to test with I2C I had to use #91664)

Helper usage:

    uint8_t regs_list[]  = { reg_addr1, reg_addr2. ... }
    struct rtio_reg_buf rbuf[] = {{mem_addr_1, mem_len_1},
                                  {mem_addr_2, mem_len_2},
                                  ...
                                 };

    rtio_read_transaction(dev,
                          regs_list,
                          ARRAY_SIZE(regs_list),
                          BUS_SPI,
                          rbuf,
                          sqe,
                          iodev,
                          rtio,
                          op_cb);

Points to be discussed:

This helper is defined as static inline. Since it may be called in more places by several drivers, this would lead to a code size increasing. Is there any other possibility?
The helper currently is declared as void. Should we consider returning an error code?
I'm handling the bus part internally, because, in my opinion, this part seems to pertain to RTIO-enabled bus. Is this matching what we have said in the sensor WG? I may have missed some comments...
This helper would fail if the rtio resources are not properly dimensioned in the DT. This is true always, even if the helper is not there. But maybe we should properly use ASSERT to help implementors to debug...

avisconti · 2025-06-20T13:06:21Z

Is this PR matching your expectation?

teburd

I think this makes sense, especially as there's already a need in some st drivers. Having it here means we can always update it with a grep and refactor if needed.

+1 from me once the function params are sorted out

include/zephyr/rtio/rtio.h

avisconti · 2025-06-24T15:03:02Z

I rebased just to have #91664 included.
Reworked the arguments order as per @teburd suggestions.

Still, I have some doubts on this point:
"This helper is defined as static inline. Since it may be called in more places by several drivers, this would lead to a code size increasing. Is there any other possibility?"
Any consideration?

teburd · 2025-06-24T15:17:20Z

I rebased just to have #91664 included. Reworked the arguments order as per @teburd suggestions.

Still, I have some doubts on this point: "This helper is defined as static inline. Since it may be called in more places by several drivers, this would lead to a code size increasing. Is there any other possibility?" Any consideration?

Try it and see, what I've seen gcc do now is intelligently generate symbols for these functions when they don't meet the inline criteria seemingly, and otherwise inline them. So, I believe (and please tell me I'm wrong if so!) that the compiler inline heuristics are deciding what to inline and when rather than explicitly forcing a function call or always inlining (ALWAYS_INLINE isn't used here).

In theory static inline + LTO should be smart and generate symbols per translation unit based on the inlining heuristics, then de-duplicate across the program when linking.

At least that is the intent here, but C is weird with this stuff so if that's not the case then I think you are correct that this should probably be a real symbol and function call and all of that.

Makes me wonder if we should explore a unity build of zephyr at some point to avoid a lot of the oddities around translation units, inlines, and link time optimizations... but that would probably be very difficult. https://en.wikipedia.org/wiki/Unity_build

avisconti · 2025-06-24T16:17:34Z

I rebased just to have #91664 included. Reworked the arguments order as per @teburd suggestions.
Still, I have some doubts on this point: "This helper is defined as static inline. Since it may be called in more places by several drivers, this would lead to a code size increasing. Is there any other possibility?" Any consideration?

Try it and see, what I've seen gcc do now is intelligently generate symbols for these functions when they don't meet the inline criteria seemingly, and otherwise inline them. So, I believe (and please tell me I'm wrong if so!) that the compiler inline heuristics are deciding what to inline and when rather than explicitly forcing a function call or always inlining (ALWAYS_INLINE isn't used here).

In theory static inline + LTO should be smart and generate symbols per translation unit based on the inlining heuristics, then de-duplicate across the program when linking.

At least that is the intent here, but C is weird with this stuff so if that's not the case then I think you are correct that this should probably be a real symbol and function call and all of that.

Makes me wonder if we should explore a unity build of zephyr at some point to avoid a lot of the oddities around translation units, inlines, and link time optimizations... but that would probably be very difficult. https://en.wikipedia.org/wiki/Unity_build

Exploring symbols and assembly code with objdump it seems to me that gcc has generated a routine indeed:

08004d94 <rtio_read_transaction.constprop.0>:
 8004d94:>      e92d 4ff7 >     stmdb>  sp!, {r0, r1, r2, r4, r5, r6, r7, r8, r9, sl, fp, lr}
 8004d98:>      469b      >     mov>    fp, r3
 8004d9a:>      e9dd a90c >     ldrd>   sl, r9, [sp, #48]>      ; 0x30
 8004d9e:>      9b0e      >     ldr>    r3, [sp, #56]>  ; 0x38
 8004da0:>      4606      >     mov>    r6, r0
 8004da2:>      9300      >     str>    r3, [sp, #0]
 8004da4:>      9b0f      >     ldr>    r3, [sp, #60]>  ; 0x3c

Called in multiple places:

 8005116:>      f7ff fe3d >     bl>     8004d94 <rtio_read_transaction.constprop.0>
 800511a:>      e6e2      >     b.n>    8004ee2 <lsm6dsv16x_read_fifo_cb+0x76>
 800511c:>      0800b28b >      stmdaeq>r0, {r0, r1, r3, r7, r9, ip, sp, pc}

So it seems correct. Thanks for explanation.

ubieda

Just dropping a drive-by comment (I can take a look once I'm back at the office next week, unless it's merged before):

I think we'll want to come back later and refactor this when we do introduce the regmap APIs.

Otherwise, I agree this helper is used in many places and it's useful.

kartben · 2025-06-27T13:00:50Z

include/zephyr/rtio/rtio.h

+static inline bool rtio_is_spi(rtio_bus_type bus_type)
+{
+	return (bus_type == BUS_SPI);
+}
+
+static inline bool rtio_is_i2c(rtio_bus_type bus_type)
+{
+	return (bus_type == BUS_I2C);
+}
+
+static inline bool rtio_is_i3c(rtio_bus_type bus_type)
+{
+	return (bus_type == BUS_I3C);
+}


please add doxygen comments ; also, mention all these new API in release notes

OK, but these are meant to be only used internally here. But I can make them as APIs in fact. Good

ubieda · 2025-06-27T12:36:21Z

include/zephyr/rtio/rtio.h

+ * @brief A structure to describe a list of not-consecutive memory chunks
+ * for RTIO operations.
+ */
+struct rtio_reg_buf {


Let's consider moving these additions to a separate header file (e.g: include/zephyr/rtio/regmap.h perhaps?)

Do you mean only this one, or ALL the inline additions as well?

OK, I'll move ALL stuff in regmap.h

ubieda · 2025-06-27T12:40:02Z

include/zephyr/rtio/rtio.h

+ * @param r RTIO context
+ * @param rtio_callback_t callback routine at the end of op
+ */
+static inline void rtio_read_transaction(struct rtio *r,


Since "RTIO Transaction" is an already defined term, I'd suggest naming this something else that better reflects the fact that we're doing multiple register reads (chained write-read transactions). For instance:

Suggested change

static inline void rtio_read_transaction(struct rtio *r,

static inline void rtio_read_regs_async(struct rtio *r,

seems a good name

ubieda · 2025-06-27T13:49:18Z

include/zephyr/rtio/rtio.h

+		}
+
+		if (rtio_is_spi(bus_type)) {
+			regs[i] |= 0x80; /* mark the SPI read transaction */


I feel like this should be configurable: there are some drivers that have this inverted (as in: 1 when writing, 0 when reading).

Here's one in-tree example:

zephyr/drivers/sensor/pixart/pat9136/pat9136_reg.h

Lines 11 to 12 in 4ec4351

#define REG_SPI_READ_BIT 0

#define REG_SPI_WRITE_BIT BIT(7)

I see. Yes, I remember now that you mentioned it during the WG call, but at the end I forgot it. So, the best thing to do is that the driver would raise the proper bit in regs_list[].reg_addr (regs_list is the second argument)

dkalowsk · 2025-06-27T18:58:44Z

@avisconti please address the issues raised by @kartben and @ubieda so we can merge this for 4.2

avisconti · 2025-06-28T15:33:25Z

@avisconti please address the issues raised by @kartben and @ubieda so we can merge this for 4.2

Sorry, yesterday I was in vacation. I'll do it right now

avisconti · 2025-06-28T19:40:21Z

I included all suggestions from @ubieda and @kartben review.
I'm not sure we are still in time to include it in 4.2 (unfortunately yesterday I had to be on leave). Moreover I was not able to check if it is still working properly, as I do not have my setup at home. I have checked building though. In any case I could fix any possible mistake in the next weeks. Feel free to evaluate whether it is too risky to merge it in 4.2 (provided that we are still in time)

I appreciated your review guys. So sorry I couldn't work on it Friday

Add a helper function that constructs a rtio SQE chain with the purpose to perform a bus read operation on a list of registers. Usage: uint8_t regs_list[] = { reg_addr1, reg_addr2. ... } struct rtio_reg_buf rbuf[] = {{mem_addr_1, mem_len_1}, {mem_addr_2, mem_len_2}, ... }; rtio_read_transaction(dev, regs_list, ARRAY_SIZE(regs_list), BUS_SPI, rbuf, sqe, iodev, rtio, op_cb); Signed-off-by: Armando Visconti <armando.visconti@st.com>

sonarqubecloud · 2025-06-28T21:15:18Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

avisconti added Enhancement Changes/Updates/Additions to existing features area: Drivers area: Sensors Sensors area: RTIO labels Jun 17, 2025

github-actions bot requested review from asemjonovs, jeppenodgaard, MaureenHelm, teburd, tristan-google, ubieda and yperess June 17, 2025 12:34

github-actions bot assigned MaureenHelm Jun 17, 2025

avisconti force-pushed the add-rtio-rd-helper branch 2 times, most recently from 240bd24 to d253799 Compare June 17, 2025 13:33

avisconti mentioned this pull request Jun 17, 2025

Add read_and_decode API based iis3dwb sensor driver #90286

Open

teburd reviewed Jun 23, 2025

View reviewed changes

include/zephyr/rtio/rtio.h Outdated Show resolved Hide resolved

avisconti force-pushed the add-rtio-rd-helper branch from d253799 to d644b5c Compare June 24, 2025 15:00

avisconti requested a review from teburd June 24, 2025 15:03

teburd previously approved these changes Jun 24, 2025

View reviewed changes

ubieda reviewed Jun 24, 2025

View reviewed changes

kartben reviewed Jun 27, 2025

View reviewed changes

ubieda reviewed Jun 27, 2025

View reviewed changes

MaureenHelm previously approved these changes Jun 27, 2025

View reviewed changes

avisconti dismissed MaureenHelm’s stale review via 9d5201f June 28, 2025 19:36

avisconti dismissed teburd’s stale review via 9d5201f June 28, 2025 19:36

avisconti force-pushed the add-rtio-rd-helper branch from d644b5c to 9d5201f Compare June 28, 2025 19:36

avisconti requested review from kartben, teburd, ubieda and MaureenHelm June 28, 2025 19:36

github-actions bot added the Release Notes To be mentioned in the release notes label Jun 28, 2025

github-actions bot requested review from danieldegrasse and dkalowsk June 28, 2025 19:37

avisconti force-pushed the add-rtio-rd-helper branch from 9d5201f to 6b38cbe Compare June 28, 2025 20:49

ubieda approved these changes Jun 30, 2025

View reviewed changes

teburd approved these changes Jun 30, 2025

View reviewed changes

MaureenHelm approved these changes Jun 30, 2025

View reviewed changes

kartben added this to the v4.3.0 milestone Jul 4, 2025

	static inline void rtio_read_transaction(struct rtio *r,
	static inline void rtio_read_regs_async(struct rtio *r,

rtio: add helper function rtio_read_transaction() #91775

Are you sure you want to change the base?

rtio: add helper function rtio_read_transaction() #91775

Conversation

avisconti commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avisconti commented Jun 20, 2025

Uh oh!

teburd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

avisconti commented Jun 24, 2025

Uh oh!

teburd commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avisconti commented Jun 24, 2025

Uh oh!

ubieda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkalowsk commented Jun 27, 2025

Uh oh!

avisconti commented Jun 28, 2025

Uh oh!

avisconti commented Jun 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Jun 28, 2025

Quality Gate passed

Uh oh!

Uh oh!

rtio: add helper function `rtio_read_transaction()` #91775

rtio: add helper function `rtio_read_transaction()` #91775

avisconti commented Jun 17, 2025 •

edited

Loading

teburd commented Jun 24, 2025 •

edited

Loading

avisconti commented Jun 28, 2025 •

edited

Loading